Skip to content

feat(server): SDK-owned resource subscription tracking on McpServer#2440

Open
felixweinberger wants to merge 1 commit into
mainfrom
fweinberger/resource-subscription-tracking
Open

feat(server): SDK-owned resource subscription tracking on McpServer#2440
felixweinberger wants to merge 1 commit into
mainfrom
fweinberger/resource-subscription-tracking

Conversation

@felixweinberger

@felixweinberger felixweinberger commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

A server that declares resources: { subscribe: true } gets no help from the high-level API today: it must reach through mcpServer.server to register the resources/subscribe and resources/unsubscribe handlers, keep its own Set of subscribed URIs, and call server.server.sendResourceUpdated, because the facade offers sendResourceListChanged but no updated sibling. Every consumer that serves 2025-era sessions rewrites the same ~20 lines (the resources example and todos-server both did). The go and java SDKs own this bookkeeping in the SDK; on 2026-07-28 connections our serving entries already own the equivalent via the subscriptions/listen router — this brings the 2025-era path to parity.

Declaring the capability is now enough. At McpServer.connect(), a declared resources.subscribe capability auto-installs the two request handlers when neither verb has one yet: subscribes are recorded in the per-connection read-only mcpServer.resourceSubscriptions set (cleared on every connect), unsubscribes remove them, and sendResourceUpdated completes the notification facade. The install is deliberately deferred to connect so hand-registered handlers win by existing — servers using the low-level escape hatch keep their exact behavior, nothing throws at startup, and the auto arm registers no capabilities so it cannot fail for any pre-existing app.

trackResourceSubscriptions({ onSubscribe, onUnsubscribe }) is the explicit variant for servers that need veto hooks: the hook runs before the set changes and a throw refuses the request (propagated as the JSON-RPC error, with an abort re-check after the await so a cancelled request never records). It must be called before the first connect().

Motivation and Context

See above — the resources guide recipe and both in-repo consumers hand-rolled identical bookkeeping; the SDK now owns the mechanics while the app keeps the policy (veto). Auto-on was a deliberate call for the v2 window, with the behavior change documented in the migration guide.

How Has This Been Tested?

Seventeen tests over linked client/server pairs: the auto path (declared capability → subscribe/unsubscribe work at connect, URIs recorded/removed); no capability and no call → -32601 unchanged; a hand-registered handler wins over the auto-install (manual response observed, SDK set stays empty); auto-tracking over low-level servers that answer list/read themselves; veto on subscribe and unsubscribe (client receives the error, set unchanged); a request cancelled during the hook does not record; sendResourceUpdated delivers notifications/resources/updated; duplicate explicit call throws; capability auto-declaration and merge on the explicit path; post-connect explicit call throws; conflict atomicity (no capability half-declared); reconnect idempotence (no duplicate-handler throw, set reset each connect). Full workspace suite: server 417, integration 348, e2e 2633 (+155 pre-existing expected-fail), all packages green; docs gates clean; live probes of all four auto semantics through a real client.

Breaking Changes

One behavioral change, documented in docs/migration/upgrade-to-v2.md: servers that declared resources: { subscribe: true } but registered no handlers previously advertised the capability while answering -32601 Method not found; they now get working SDK-owned subscription semantics. Hand-registered handlers are unaffected. Tests pinning -32601 for that misconfiguration need re-baselining.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

An earlier revision shipped this as explicit opt-in; it was promoted to capability-driven auto-enable during review (the v2 launch window permits the behavior change, and the connect-time deferral makes it conflict-free). The examples simplify to a constructor capability declaration with no explicit call; the docs recipe leads with the auto path and shows trackResourceSubscriptions for the veto-hook variant. Protocol.hasRequestHandler(method) is added as the public probe the auto-install needs (and assertCanSetRequestHandler now delegates to it). One sharp edge, documented: calling trackResourceSubscriptions() after a connect has auto-installed throws the duplicate-registration error — attach hooks before the first connect.

@felixweinberger felixweinberger requested a review from a team as a code owner July 6, 2026 15:39
@changeset-bot

changeset-bot Bot commented Jul 6, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: ab94a4b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages
Name Type
@modelcontextprotocol/server Minor
@modelcontextprotocol/express Major
@modelcontextprotocol/fastify Major
@modelcontextprotocol/hono Major
@modelcontextprotocol/node Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@pkg-pr-new

pkg-pr-new Bot commented Jul 6, 2026

Copy link
Copy Markdown

Open in StackBlitz

@modelcontextprotocol/client

npm i https://pkg.pr.new/@modelcontextprotocol/client@2440

@modelcontextprotocol/codemod

npm i https://pkg.pr.new/@modelcontextprotocol/codemod@2440

@modelcontextprotocol/core

npm i https://pkg.pr.new/@modelcontextprotocol/core@2440

@modelcontextprotocol/server

npm i https://pkg.pr.new/@modelcontextprotocol/server@2440

@modelcontextprotocol/server-legacy

npm i https://pkg.pr.new/@modelcontextprotocol/server-legacy@2440

@modelcontextprotocol/express

npm i https://pkg.pr.new/@modelcontextprotocol/express@2440

@modelcontextprotocol/fastify

npm i https://pkg.pr.new/@modelcontextprotocol/fastify@2440

@modelcontextprotocol/hono

npm i https://pkg.pr.new/@modelcontextprotocol/hono@2440

@modelcontextprotocol/node

npm i https://pkg.pr.new/@modelcontextprotocol/node@2440

commit: ab94a4b

Comment thread packages/server/src/server/mcp.ts
Comment thread docs/servers/resources.md Outdated
Comment thread examples/todos-server/todos.ts
@felixweinberger felixweinberger force-pushed the fweinberger/resource-subscription-tracking branch from 8cb0563 to 26fc34c Compare July 6, 2026 17:16
Comment on lines +590 to +598
this.server.setRequestHandler('resources/subscribe', async (request, ctx) => {
await options?.onSubscribe?.(request.params.uri, ctx);
// A request cancelled while the hook ran must not mutate the set —
// the protocol suppresses the aborted request's response, so the
// client would never learn a subscription was recorded.
ctx.mcpReq.signal.throwIfAborted();
this._resourceSubscriptions.add(request.params.uri);
return {};
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 The resources/subscribe handler installed by trackResourceSubscriptions() adds the client-supplied URI to _resourceSubscriptions with no bound on set size or URI length, so a misbehaving 2025-era client on a long-lived sessionful connection can grow server memory without limit — unlike the SDK's own 2026-era listen router, which caps its analog at DEFAULT_MAX_SUBSCRIPTIONS = 1024. Answering the open question in the PR description: yes, a default cap (with an override, mirroring the maxSubscriptions option on serveStdio/createMcpHandler) would be worth adding for symmetry — or at minimum, document using onSubscribe to enforce a bound, since none of the in-repo consumers or docs recipes pass one.

Extended reasoning...

What this is. The resources/subscribe handler installed by trackResourceSubscriptions() (packages/server/src/server/mcp.ts:590-598) does an unconditional this._resourceSubscriptions.add(request.params.uri). The URI is entirely client-controlled input: nothing bounds the set size, nothing bounds the URI length, and nothing requires the URI to correspond to a registered resource (the handler accepts any string). On a sessionful posture (stdio, sessionful Streamable HTTP) the set lives for the lifetime of the connection, so an untrusted or buggy 2025-era client can send an unbounded stream of resources/subscribe requests with unique, arbitrarily long URIs and grow server memory without limit for as long as the session stays open.

Why nothing else prevents it. assertRequestHandlerCapability has no case for resources/subscribe, and the only mitigation the API offers is the optional onSubscribe veto hook — which every consumer this PR ships leaves unset: examples/resources/server.ts, examples/todos-server/todos.ts, the rewritten recipe in docs/servers/resources.md, and the JSDoc example all call trackResourceSubscriptions() bare (the JSDoc example does show a hook, but for URI-prefix filtering, not a count bound). So the default configuration — the one the docs teach — has no guard at all.

The in-package precedent. The SDK's own 2026-era analog treats exactly this class of client-driven bookkeeping as something that must be bounded: packages/server/src/server/listenRouter.ts:36 defines DEFAULT_MAX_SUBSCRIPTIONS = 1024, both createListenRouter (line ~124) and StdioListenRouter (line ~323) refuse further listens past the cap with a -32603 "Subscription limit reached" error, and both serveStdio and createMcpHandler expose this as a maxSubscriptions option defaulting to 1024. Now that the SDK owns the 2025-era bookkeeping too, the same protection belongs here.

Concrete walk-through. 1) A server calls server.trackResourceSubscriptions() (no options) and serves a sessionful stdio or Streamable HTTP connection. 2) A client sends resources/subscribe with params.uri = 'x://' + 'a'.repeat(1_000_000) + i in a loop, incrementing i. 3) Each request passes straight through the handler (no capability gate for the verb, no registered-resource check, no hook), executes _resourceSubscriptions.add(uri), and returns {}. 4) After N requests the Set holds N megabyte-scale strings; nothing evicts them until the connection closes. The equivalent loop against a 2026-era subscriptions/listen connection is refused after 1024 entries by the listen router.

Addressing the counter-argument. One reviewer position is that this merely restates the author's own open question ("Also open to adding a cap on the set... if reviewers want symmetry") and that the pre-PR hand-rolled Sets in the docs and examples had the identical exposure, so nothing regresses. Both points are true — which is exactly why this is filed as a nit rather than a blocker. But the question was explicitly left for reviewers to answer, and moving the bookkeeping from app code into the SDK changes who is responsible for the bound: an app hand-rolling a Set can be expected to own its own limits, while an SDK-owned default should be safe out of the box, the way the listen router already is. So the answer to the open question: yes, add a default cap.

How to fix. Either (a) accept a maxSubscriptions option on trackResourceSubscriptions defaulting to 1024 (matching the listen router's constant and error behavior — refuse with a ProtocolError once _resourceSubscriptions.size >= max), or (b) at minimum, extend the JSDoc/docs recipe to show an onSubscribe hook that enforces a count bound, so the documented default posture isn't unbounded. Option (a) is preferable for symmetry and because it protects the callers who never read that paragraph. This is not a merge blocker: no correct client hits it, and it is not a regression relative to the pattern the PR replaces.

An app that declares resources.subscribe today gets no help from the
high-level server: it must reach through mcpServer.server to register
the resources/subscribe and resources/unsubscribe handlers, keep its
own Set of subscribed URIs, and call server.server.sendResourceUpdated,
because the facade has sendResourceListChanged but no updated sibling.

Declaring the resources.subscribe capability is now the whole opt-in:
at connect() time, McpServer installs the resources/subscribe and
resources/unsubscribe handlers when neither verb has one yet, and
records subscribed URIs in the new read-only per-connection
resourceSubscriptions set. Deferring the install to connect keeps
hand-registered handlers (the previous escape-hatch pattern) winning by
already existing, so servers that answer the verbs themselves keep
their exact behavior; the install is skipped as a pair. connect()
resets the set, so a reused instance cannot leak one connection's
subscriptions into the next, and repeat connects find the first
connect's handlers and only clear the set.

trackResourceSubscriptions({ onSubscribe?, onUnsubscribe? }) is the
explicit configuration path: called before connect it declares the
capability, installs the handlers, and attaches veto hooks that run
before the set changes — a hook throw is returned to the client as the
request's error, and a request cancelled while a hook runs leaves the
set unchanged (the handlers re-check the abort signal). The method is
assert-first: a duplicate call, a manually installed subscribe handler,
or a conflicting low-level resources/list handler throws before any
capability or handler state changes. A sendResourceUpdated facade
completes the notification trio, and Protocol gains a public
hasRequestHandler(method) accessor backing the connect-time check.

On 2026-07-28 connections the two verbs do not exist (clients filter
subscriptions/listen and the serving entries own that bookkeeping), and
the era gate rejects them before any handler runs, so the automatic
install is harmless on dual-era servers.

Apps that declared resources.subscribe without registering handlers
previously advertised the capability while answering -32601; they now
get working subscribe/unsubscribe — documented in the migration guide.
The resources and todos-server examples and the resources guide recipe
now rely on the automatic path; the low-level guide points readers at
it for this pair.
@felixweinberger felixweinberger force-pushed the fweinberger/resource-subscription-tracking branch from 26fc34c to ab94a4b Compare July 6, 2026 19:05
@felixweinberger felixweinberger changed the title feat(server): opt-in resource subscription tracking on McpServer feat(server): SDK-owned resource subscription tracking on McpServer Jul 6, 2026
Comment on lines 149 to +157
async connect(transport: Transport): Promise<void> {
// A declared `resources.subscribe` capability gets SDK-owned subscription
// bookkeeping automatically; deferring the install to connect time lets a
// hand-registered `resources/subscribe` handler win by already existing.
this._autoTrackResourceSubscriptions();
// Per-resource subscriptions are connection state: a reused instance must
// not carry the previous connection's subscriptions onto a new transport,
// where they would route unsolicited `notifications/resources/updated`.
this._resourceSubscriptions.clear();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 The PR description says this ships the explicit opt-in form ("zero behavior change for existing servers", "Breaking Changes: None — the API is opt-in; without it, behavior is byte-for-byte today's"), but the diff ships automatic activation: connect() calls _autoTrackResourceSubscriptions(), which installs the resources/subscribe/resources/unsubscribe handlers for any server that merely declared resources: { subscribe: true } — a behavior change the changeset and migration guide already document. Please update the description (and the Breaking Changes / types-of-changes framing) to reflect the automatic activation, so the auto-vs-explicit design decision — the one the description says was "deliberately left open" — gets reviewed as what actually shipped.

Extended reasoning...

What the mismatch is. The PR description's "Additional context" section states: "One design question deliberately left open for review: whether declaring resources: { subscribe: true } should enable tracking automatically instead of requiring the explicit call. This PR ships the explicit form — zero behavior change for existing servers", and the Breaking Changes section says "None — the API is opt-in; without it, behavior is byte-for-byte today's." The code in the diff does the opposite: McpServer.connect() (packages/server/src/server/mcp.ts:149-157) unconditionally calls _autoTrackResourceSubscriptions(), and that helper (~lines 645-655) installs the resources/subscribe and resources/unsubscribe handlers for any server whose declared capabilities include resources.subscribe — no explicit trackResourceSubscriptions() call required.

Concrete walk-through. 1) An existing app constructs new McpServer({...}, { capabilities: { resources: { subscribe: true } } }) and registers no subscribe handlers (a common posture today — the capability was declared, the verbs answered -32601). 2) On this branch, connect() runs _autoTrackResourceSubscriptions(); the capability check passes, neither verb has a handler, so _installResourceSubscriptionHandlers() runs. 3) A client's resources/subscribe now returns {} and the URI is recorded in resourceSubscriptions, where before it returned -32601 Method not found. That is precisely the automatic form the description claims was not shipped, and precisely the "behavior change for existing servers" it claims doesn't exist. The new test 'a declared resources.subscribe capability auto-installs tracking at connect' pins this behavior explicitly.

The rest of the PR agrees with the code, not the description. The changeset (.changeset/track-resource-subscriptions.md) says "Declaring the resources.subscribe capability now activates it automatically at connect()" and carries an explicit "Behavior change:" paragraph (previously -32601, now working SDK-owned handlers). The migration guide (docs/migration/upgrade-to-v2.md) adds a matching bullet, and docs/servers/resources.md is rewritten around "Declaring resources: { subscribe: true } is the whole opt-in." So the shipped artifacts — code, changeset, migration guide, docs, tests — are all internally consistent; only the PR description contradicts them. (The "Ten new tests" count is also stale; the test file has 13.) The description evidently predates the commit that graduated the feature from explicit to automatic and was never refreshed.

Why it matters. The description misleads reviewers on exactly the design question it flags as "deliberately left open for review": a reviewer reading only the description would believe the automatic-activation decision is still pending and that existing capability-declaring servers are unaffected, when in fact both have been decided in the code. It also means the "Breaking change" checkbox / "non-breaking" framing doesn't match the documented -32601{} behavior change for existing servers, which is worth a deliberate maintainer sign-off (including whether a "minor" changeset is the right vehicle for it).

How to fix. Update the PR description: state that the automatic form ships (declaring the capability alone activates tracking at connect(), hand-registered handlers still win), revise the Breaking Changes / Types-of-changes sections to acknowledge the documented behavior change for servers that declared resources.subscribe without handlers, and refresh the test count. Alternatively, if the maintainers prefer the explicit-only design the description describes, drop the _autoTrackResourceSubscriptions() call from connect() — but then the changeset, migration guide, docs, and tests all need reverting to match. Nothing in the merged code is wrong or self-inconsistent, so this is not a merge blocker — it's stale review metadata that should be corrected so the design decision is reviewed as what it actually is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant